-
Notifications
You must be signed in to change notification settings - Fork 902
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement Series.datetime.floor #9488
Implement Series.datetime.floor #9488
Conversation
Codecov Report
@@ Coverage Diff @@
## branch-21.12 #9488 +/- ##
================================================
- Coverage 10.79% 10.66% -0.13%
================================================
Files 116 117 +1
Lines 18869 19721 +852
================================================
+ Hits 2036 2104 +68
- Misses 16833 17617 +784
Continue to review full report at Codecov.
|
rerun tests |
) | ||
], | ||
) | ||
@pytest.mark.parametrize("time_type", DATETIME_TYPES) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Docs say Timedelta
is supported too, add those tests separately in test_timedelta.py
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, could you also take a look at the comment above here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@galipremsagar based on slack conversation with @shwina, I have updated the docstrings to state that it only returns Series
for now. Adding floor and ceil methods to DatetimeIndex
should be a separate PR, with no changes required on the libcudf
side. However, some work on the C++ side will be required to support TimeDeltaIndex
.
Sample error:
>>> import pandas
>>> import cudf
>>> data = ['10 day 5h 2 min 3us 10ns',
... '+22:39:19.999999',
... '2 day 4h 03:08:02.000045',
... '+21:15:45.999999']
>>> dtype = "timedelta64[ns]"
>>> psr = pandas.TimedeltaIndex(data, dtype=dtype)
>>> gsr = cudf.TimedeltaIndex(data, dtype=dtype)
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "/home/u00ubzohp7sU4seIFv357/dev/rapids/cudf/python/cudf/cudf/core/index.py", line 1931, in __init__
data = column.as_column(np.array(data, dtype=dtype))
ValueError: Could not convert object to NumPy timedelta
Thanks to both of you for catching and clarifying this!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not have support for datetime64 types(Both Series & Index) in this PR and have timedelta64 added as follow-up ? I don't understand why we choose to make a separate PR: #9554
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this came up as a recommendation from @shwina, so he might be the best person to answer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought a separate PR would be appropriate as this PR didn't touch anything ceil
related. Apologies for any confusion.;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mayankanand007 Can you create issues for ceil
& floor
support for timedelta64
types too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cython/Python approval.
@gpucibot merge |
This reverts commit 201f750.
Fixes: #7102 Replaces: [#9488](https://github.com/rapidsai/cudf/pull/9488/files) Authors: - Sheilah Kirui (https://github.com/skirui-source) - Mayank Anand (https://github.com/mayankanand007) Approvers: - GALI PREM SAGAR (https://github.com/galipremsagar) - Michael Wang (https://github.com/isVoid) - Bradley Dice (https://github.com/bdice) - Vyas Ramasubramani (https://github.com/vyasr) URL: #9571
Fixes: #7102